Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jamtis changes #26

Open
wants to merge 36 commits into
base: seraphis_lib
Choose a base branch
from
Open

Conversation

jeffro256
Copy link

  • nominal address tag protection for LWs (extra address key)
  • flexible view tags
  • churning and pocketchange protection for LWs (auxiliary enote records)

@jeffro256 jeffro256 marked this pull request as draft December 3, 2023 16:45
@jeffro256 jeffro256 force-pushed the jamtis_lw_take_2 branch 3 times, most recently from 55cad5e to 3cf1863 Compare December 4, 2023 15:04
@jeffro256 jeffro256 marked this pull request as ready for review December 6, 2023 05:19
@jeffro256
Copy link
Author

For unmodified seraphis_lib, running the test_remote_scanner_client_scan_sp performance test, I got the following result:

test_remote_scanner_client_scan_sp (23 calls) - OK: 13391 µs/call
test_remote_scanner_client_scan_sp (23 calls) - OK: 13478 µs/call
test_remote_scanner_client_scan_sp (23 calls) - OK: 14521 µs/call

On this branch, I get:

test_remote_scanner_client_scan_sp (23 calls) - OK: 1790 ms/call
test_remote_scanner_client_scan_sp (23 calls) - OK: 1811 ms/call
test_remote_scanner_client_scan_sp (23 calls) - OK: 1802 ms/call

This means that for the different test modes ALL_FAKE, ONE_FAKE_TAG_MATCH, ONE_OWNED, the new client-side view scanning takes 134x, 134x, and 124x, respectively, times more CPU cycles than unmodified seraphis_lib. This puts it on par with the dense/sparse changes #15.

@jeffro256
Copy link
Author

Latest commit adds a unit test case which performs a successful Janus attack with knowledge of one address private key. Will push fix soon...

@jeffro256
Copy link
Author

Currently making modifications to the "Implementing Seraphis" document to reflect the changes in this PR. Will release a PR to the Seraphis repo when done.

jeffro256 added a commit to jeffro256/Seraphis that referenced this pull request Dec 21, 2023
* nominal address tag protection for LWs (extra address key)
* flexible view tags
* churning and pocketchange protection for LWs (auxiliary enote records)

PR implementing changes is here: UkoeHB/monero#26
@jeffro256
Copy link
Author

Jamtis changes are documented here: UkoeHB/Seraphis#6

@UkoeHB UkoeHB force-pushed the seraphis_lib branch 2 times, most recently from d493a6d to 279be2b Compare February 9, 2024 00:11
@jeffro256 jeffro256 force-pushed the jamtis_lw_take_2 branch 3 times, most recently from af37a83 to 5ecea4f Compare February 14, 2024 19:01
@jeffro256
Copy link
Author

Rebased

@jeffro256 jeffro256 force-pushed the jamtis_lw_take_2 branch 2 times, most recently from f73feab to 1761394 Compare February 14, 2024 19:05
@UkoeHB
Copy link
Owner

UkoeHB commented Feb 22, 2024

Needs to be updated to reflect the notational changes made in UkoeHB/Seraphis#6.

@jeffro256
Copy link
Author

Did a simple rebase again to fix merge conflicts with the derived view balance key PR. Will tackle updating notation today

@jeffro256
Copy link
Author

How does the notation look on first glance?

Copy link
Owner

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

13/72 files done.

One thing you need to do is check that all the KDF transcripts are <= 128 bytes (i.e. one blake2b block). If > 128 then the cost to hash increases a lot. You can test this by editing SpKDFTranscript so the constructor prints the domain separator, and the .size() method prints its size. Then when you run the seraphis unit test suite all the sizes will print.

If you find any that crept over the line, then the relevant domain separator needs to be shaved down.

src/crypto/x25519.h Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_account_secrets.h Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_account_secrets.h Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_account_secrets.h Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_address_tag_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.h Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
const SpTxSupplementV1 &tx_supplement,
const std::vector<SpEnoteVariant> &enotes_in_tx,
std::vector<bool>& is_auxiliary_out);
bool try_find_sp_enotes_in_tx(const crypto::x25519_secret_key &d_filter_assist,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind there being two try_find_sp_enotes_in_tx is that the first one does the meat of the work, but does the least amount of allocations and copying possible. All it really needs to allocate is the output vector of bools. This ideally would be used in actual production code which scans on behalf of users, but doesn't need to send out chunk data yet, and just caches some bools persistently (maybe as unsigned ints for compactness).

src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
tests/unit_tests/seraphis_knowledge_proofs.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_enote_utils.cpp Outdated Show resolved Hide resolved
@jeffro256
Copy link
Author

In regards to the KDF transcripts being smaller than 128 bytes, I applied this diff:

diff --git a/src/seraphis_crypto/sp_transcript.h b/src/seraphis_crypto/sp_transcript.h
index 17db163d6..f0e192fb3 100644
--- a/src/seraphis_crypto/sp_transcript.h
+++ b/src/seraphis_crypto/sp_transcript.h
@@ -100,7 +100,7 @@ public:
 
     /// access the transcript data
     const void* data() const { return m_transcript.data(); }
-    std::size_t size() const { return m_transcript.size(); }
+    std::size_t size() const { if (m_transcript.size() > 128) { std::cout << "BIG TRANSCRIPT: " << m_transcript.data(); } return m_transcript.size(); }
 
 private:
 //member variables

I then ran ./build/Linux/jamtis_lw_take_2/debug/tests/unit_tests/unit_tests --gtest_filter=seraphis* | grep jamtis_. The only domain separator that showed up was jamtis_input_context_standardlegacy_input_KI, so I don't think that these changes should've made any transcripts go over 128 bytes.

Copy link
Owner

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 2 (20/72 files done)

src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_impl/serialization_demo_types.h Outdated Show resolved Hide resolved
src/seraphis_impl/serialization_demo_types.h Outdated Show resolved Hide resolved
src/seraphis_impl/serialization_demo_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_impl/serialization_demo_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_main/enote_record_types.h Outdated Show resolved Hide resolved
@jeffro256
Copy link
Author

Rebased

UkoeHB and others added 2 commits March 21, 2024 09:48
* SpTxCoinbaseV1: remove block_reward field

Not storing/serializing `block_reward` saves us a few bytes on coinbase transactions, and makes it so that you can't
initialize a coinbase transaction that has a block reward not matching its output sum.
@jeffro256
Copy link
Author

Rebased again

Copy link
Owner

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 3 (44/72 files done)

Skipping over the files with huge diffs for now.

src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_core/jamtis_payment_proposal.cpp Outdated Show resolved Hide resolved
src/seraphis_main/tx_builders_outputs.h Outdated Show resolved Hide resolved
src/seraphis_main/enote_record_types.h Outdated Show resolved Hide resolved
src/seraphis_main/tx_builders_mixed.cpp Outdated Show resolved Hide resolved
src/seraphis_main/tx_component_types.cpp Outdated Show resolved Hide resolved
src/seraphis_main/tx_component_types.cpp Outdated Show resolved Hide resolved
src/seraphis_main/tx_validators.h Outdated Show resolved Hide resolved
src/seraphis_mocks/make_mock_tx.h Outdated Show resolved Hide resolved
@DangerousFreedom1984
Copy link

Wouldn't it be better to use

  1. sr1 instead of q
  2. sr2 instead of baked_key ?

@DangerousFreedom1984
Copy link

DangerousFreedom1984 commented Mar 28, 2024

Okay, I reviewed the new changes and compared to the new seraphis draft paper and it is looking good to me. Very nice work!

@jeffro256
Copy link
Author

@DangerousFreedom1984 It might be better to use that notation in order to match the implementation paper. It also might be a little easier to read them wrong when skimming. I don't have a super strong opinion either way.

UkoeHB and others added 8 commits April 23, 2024 16:39
* direct & compact tx serialization

txs are [de]serialized directly from their classes and sizes of containers are not serialized
if they can be implied.
Implement async wallet scanner.

Adds a new functional test for direct wallet2 -> live RPC daemon
interactions. This sets up a framework to test pointing the
Seraphis wallet lib to a live daemon.

Tests sending and scanning:
- a normal transfer
- a sweep single (0 change)
- to a single subaddress
- to 3 subaddresses (i.e. scanning using additional pub keys)

* scan machine: option to force reorg avoidance increment first pass

- when pointing to a daemon that does not support returning empty
blocks when the client requests too high of a height, we have to
be careful in our scanner to always request blocks below the chain
tip, in every request.
- by forcing the reorg avoidance increment on first pass, we make
sure clients will always include the reorg avoidance increment
when requesting blocks from the daemon, so the client can expect
the request for blocks should *always* return an ok height.

* core tests: check conversion tool on all legacy enote version types

Stil TODO:
- check complete scanning on all enote types
- hit every branch condition for all enote versions

* conn pool mock: epee http client connection pool

- Enables concurrent network requests using the epee http client.
- Still TODO for production:

1) close_connections
2) require the pool respect max_connections

* enote finding context: IN LegacyUnscannedChunk, OUT ChunkData

- finds owned enotes by legacy view scanning a chunk of blocks

* async: function to remove minimum element from token queue

- Useful when we want to remove elements of the token queue in an
order that is different than insertion order.

* async scanner: scan via RPC, fetching & scanning parallel chunks

*How it works*

Assume the user's wallet must start scanning blocks from height
5000.

1. The scanner begins by launching 10 RPC requests in parallel to
fetch chunks of blocks as follows:

```
request 0: { start_height: 5000, max_block_count: 20 }
request 1: { start_height: 5020, max_block_count: 20 }
...
request 9: { start_height: 5180, max_block_count: 20 }
```

2. As soon as any single request completes, the wallet immediately
parses that chunk.
  - This is all in parallel. For example, as soon as request 7
  responds, the wallet immediately begins parsing that chunk in
  parallel to any other chunks it's already parsing.
3. If a chunk does not include a total of max_block_count blocks,
and the chunk is not the tip of the chain, this means there was a
"gap" in the chunk request. The scanner launches another parallel
RPC request to fill in the gap.
  - This gap can occur because the server will **not** return a
  chunk of blocks greater than 100mb (or 20k txs) via the
  /getblocks.bin` RPC endpoint
  ([`FIND_BLOCKCHAIN_SUPPLEMENT_MAX_SIZE`](https://github.com/monero-project/monero/blob/053ba2cf07649cea8134f8a188685ab7a5365e5c/src/cryptonote_core/blockchain.cpp#L65))
  - The gap is from `(req.start_height + res.blocks.size())` to
  `(req.start_height + req.max_block_count)`.
4. As soon as the scanner finishes parsing the chunk, it
immediately submits another parallel RPC request.
5. In parallel, the scanner identifies a user's received (and
spent) enotes in each chunk.
  - For example, as soon as request 7 responds and the wallet
  parses it, the wallet scans that chunk in parallel to any other
  chunks it's already scanning.
6. Once a single chunk is fully scanned locally, the scanner
launches a parallel task to fetch and scan the next chunk.
7. Once the scanner reaches the tip of the chain (the terminal
chunk), the scanner terminates.

*Some technical highlights*

- The wallet scanner is backwards compatible with existing daemons
(though it would need to point to an updated daemon to realize the
perf speed-up).
- On error cases such as the daemon going offline, the same wallet
errors that wallet2 uses (that the wallet API expects) are
propagated up to the higher-level Seraphis lib.
- The implementation uses an http client connection pool (reusing
the epee http client) to support parallel network requests
([related](seraphis-migration/wallet3#58)).
- A developer using the scanner can "bring their own blocks/network
implementation" to the scanner by providing a callback function of
the following type as a param to the async scanner constructor:
`std::function<bool(const cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::request, cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response)>`

---------

Co-authored-by: jeffro256 <[email protected]>
This PR removes "universal"-style indexing for legacy CLSAG rings, and replaces it with a reference set scheme that uses
(amount, index in amount) indexing pairs to reference on-chain enotes. This is the same method that Cryptonote txs use, and is how
the current Monero Core LMDB database is referenced. Doing things this way means that the database will not have to be re-indexed,
saving at a very minimum 1.6 GB (100M on-chain enotes * (16 bytes for extra table keys)) of storage space, and an expensive
database migration involving moving all existing enote data to a new table. We change the MockLedgerContext to support this indexing
scheme.

In practice, serialized txs under this method shouldn't take up much more space than pre-PR if compressed clever-ly, and assuming
most ring members will RingCT enotes.

We also add LegacyEnoteOriginContext for contextualized enote records so we can better keep tracked of scanned legacy enotes under
the legacy indexing scheme.

Co-authored-by: SNeedlewoods <[email protected]>
* nominal address tag protection for LWs (extra address key)
* flexible view tags
* churning and pocketchange protection for LWs (auxiliary enote records)
@jeffro256
Copy link
Author

Rebased to fix conflicts with #23, #39, and #42

@UkoeHB
Copy link
Owner

UkoeHB commented Aug 2, 2024

@jeffro256 Sorry the rebase broke this again. Sadly it looks like the work here may end up never being merged (largely my fault for not finishing review earlier this year).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants